Rc/7.6.0#216
Conversation
…uspiciousAppDetectionConfig
tompsota
left a comment
tompsota
left a comment
There was a problem hiding this comment.
One more thing: There are no tests for SuspiciousAppDetectionConfig, MalwareScanScope, ScopeType, or ReasonMode. Every other model in lib/src/models/ has a corresponding test/src/models/*_test.dart covering construction, toJson, fromJson, and edge cases (see test/src/models/android_config_test.dart lines 45–91 for the established pattern).
Also missing: an AndroidConfig test exercising the suspiciousAppDetectionConfig field in both fromJson/toJson directions
Can you just generate some simple test cases?
If the requested changes here are relevant also for other repos, please update those as well.
| malwareConfig: MalwareConfig( | ||
| blacklistedPackageNames: ['com.aheaditec.freeraspExample'], | ||
| suspiciousPermissions: [ | ||
| suspiciousAppDetectionConfig: SuspiciousAppDetectionConfig( |
There was a problem hiding this comment.
This is a breaking change - we need to raise major in this case
| - Android SDK version: 18.3.0 | ||
| - iOS SDK version: 6.14.4 | ||
|
|
||
| ### Breaking |
There was a problem hiding this comment.
We need to raise major because of this (on all platforms)
| required this.signingCertHashes, | ||
| this.supportedStores = const [], | ||
| this.malwareConfig, | ||
| @Deprecated('Use suspiciousAppDetectionConfig instead') this.malwareConfig, |
There was a problem hiding this comment.
Keeping both APIs is going to be huge pain. What if I specify both in config? which is going to be used?
Let's remove the old API altogether (on all platforms)
We are doing new major here so we can do this
There was a problem hiding this comment.
Did you generate this using pigeon? Or are those manual edits?
| val scopeTypeStr = optString("scanScope", "SIDELOADED_ONLY") | ||
| val scanScope = runCatching { ScopeType.valueOf(scopeTypeStr) }.getOrDefault(ScopeType.SIDELOADED_ONLY) | ||
| val trustedInstallSources = optJSONArray("trustedInstallSources")?.let { arr -> | ||
| (0 until arr.length()).map { arr.getString(it) } | ||
| } | ||
| return MalwareScanScope(scanScope, trustedInstallSources) |
There was a problem hiding this comment.
SIDELOADED_ONLY is hardcoded as a string in optString, then again as an enum in getOrDefault, then again as a fallback object — three places, two languages, no compile-time guarantee they stay in sync.
The Dart side makes scanScope required (so it can't be missing on the wire) but the Kotlin side still defends against it being absent with a string default. We need only one source of truth.
Suggested approach: just throw exception here if scope is missing.
| }.toSet() | ||
| } | ||
| val malwareScanScope = optJSONObject("malwareScanScope")?.toMalwareScanScope() | ||
| ?: MalwareScanScope(ScopeType.SIDELOADED_ONLY, emptyList()) |
There was a problem hiding this comment.
Do not fallback to default args - it creates non-debuggable issues. Just throw exception
| val packageNames = optJSONArray("packageNames")?.let { arr -> | ||
| (0 until arr.length()).map { arr.getString(it) }.toSet() | ||
| } | ||
| val hashes = optJSONArray("hashes")?.let { arr -> | ||
| (0 until arr.length()).map { arr.getString(it) }.toSet() | ||
| } | ||
| val requestedPermissions = optJSONArray("requestedPermissions")?.let { outer -> | ||
| (0 until outer.length()).map { i -> | ||
| val inner = outer.getJSONArray(i) | ||
| (0 until inner.length()).map { j -> inner.getString(j) }.toSet() | ||
| }.toSet() | ||
| } | ||
| val grantedPermissions = optJSONArray("grantedPermissions")?.let { outer -> | ||
| (0 until outer.length()).map { i -> | ||
| val inner = outer.getJSONArray(i) | ||
| (0 until inner.length()).map { j -> inner.getString(j) }.toSet() | ||
| }.toSet() | ||
| } | ||
| val malwareScanScope = optJSONObject("malwareScanScope")?.toMalwareScanScope() | ||
| ?: MalwareScanScope(ScopeType.SIDELOADED_ONLY, emptyList()) |
There was a problem hiding this comment.
The single-level and nested patterns each repeat a lot. The file already has a perfectly good generic helper for this — Utils.kt defines extractArray() and processArray() with reified types.
Can you try to reuse those ?
Also, mapTo(mutableSetOf()) avoids building a List and then copying to a Set.
|
|
||
| /// The scope of apps to be scanned for malware. | ||
| enum ScopeType { | ||
| /// Only sideloaded apps. |
There was a problem hiding this comment.
Pls remove such comments, they don;t have a value
| val malwareScanScope = optJSONObject("malwareScanScope")?.toMalwareScanScope() | ||
| ?: MalwareScanScope(ScopeType.SIDELOADED_ONLY, emptyList()) | ||
| val reasonModeStr = optString("reasonMode") | ||
| val reasonMode = if (reasonModeStr.isNullOrEmpty()) { |
There was a problem hiding this comment.
Can we set default in Dart to ReasonMode.HIGHEST_CONFIDENCE, make the argument required in kotlin and throw exception if missing?
| } | ||
| val malwareScanScope = optJSONObject("malwareScanScope")?.toMalwareScanScope() | ||
| ?: MalwareScanScope(ScopeType.SIDELOADED_ONLY, emptyList()) | ||
| val reasonModeStr = optString("reasonMode") |
There was a problem hiding this comment.
org.json.JSONObject#optString(name) returns "" (empty string) for a missing key, not null. The next line correctly uses isNullOrEmpty() so it works, but the variable name reasonModeStr plus the isNullOrEmpty() check imply the value can be null — which it can't from this API. Slightly misleading and inconsistent with the optString("scanScope", "SIDELOADED_ONLY") pattern used 30 lines above, which uses the two-arg overload to provide a default.
2 participants
No description provided.